Replace `PageRevision` with generic `Revision` model #8441

laymonage · 2022-04-27T17:06:13Z

This PR replaces the PageRevision model with a more generic Revision model. Instead of using a ForeignKey directly to the Page model, we're using a CharField object_id and a couple of ForeignKeys to the ContentType model. We are not using the GenericForeignKey field due to complexities it adds when handling the default/specific Page models (see also ticket 16055 in Django). However, we try to simulate the same behaviour as the GenericForeignKey wherever possible.

Please check the following:

Do the tests still pass?¹
Does the code comply with the style guide?
- Run make lint from the Wagtail root.
For Python changes: Have you added tests to cover the new/fixed behaviour?
For front-end changes: Did you test on all of Wagtail’s supported environments?²
- Please list the exact browser and operating system versions you tested:
- Please list which assistive technologies ³ you tested:
For new features: Has the documentation been updated accordingly?

Please describe additional details for testing this change.

squash-labs · 2022-04-27T17:06:20Z

Manage this branch in Squash

Test this branch here: https://laymonagegeneric-revision-ais9h.squash.io

laymonage

I've added some initial updates to the docs. Might need to get a better decision on the page property, though. Kindly review @kaedroho, thanks!

wagtail/admin/views/home.py

wagtail/management/commands/create_log_entries_from_revisions.py

wagtail/models/__init__.py

laymonage · 2022-04-29T16:23:13Z

wagtail/models/__init__.py

+    @cached_property
+    def page(self):
+        return self.content_object
+
+    @cached_property
+    def page_id(self):
+        return int(self.object_id)


We should either get rid of this now or add some documentation and deprecation warning and get rid of this later. Or maybe leave this in as a shorthand, but make it undocumented?

I haven't got a strong opinion on this. @gasman what do you think?

I'd go for the deprecation. It seems like something people are likely to be using in the wild, but that we don't want to keep around indefinitely.

Should this be removed in Wagtail 5.0, or 6.0?

Let's say 5.0, since we don't know at this point how far into the future 5.0 and 6.0 are going to be. (I know this tripped us up with use_json_field, and we're still figuring things out - but I think we can make it work as long as we hold off on doing the actual removals for version N until version N-1 has been released and bakerydemo is updated accordingly).

Okay, thanks! I've added the deprecation warnings. Let me know if there are any changes needed.

laymonage · 2022-04-29T16:27:07Z

wagtail/migrations/0071_populate_revision_content_type.py

+    Revision.objects.all().update(
+        base_content_type=page_type,
+        content_type_id=Cast(
+            KeyTextTransform("content_type", models.F("content")),


It turns out we can't cast jsonb into integer directly. We need to extract the value as text using the ->> operator before casting it. So, instead of doing content__content_type (which implicitly use the KeyTransform's -> operator), we should use KeyTextTransform, which extracts the value using the ->> operator.

See:

https://stackoverflow.com/questions/52457637/postgres-cannot-cast-type-jsonb-to-integer

https://stackoverflow.com/questions/25809744/cast-from-json-to-int-not-working

https://stackoverflow.com/questions/24826385/how-to-convert-postgresql-9-4s-jsonb-type-to-float

Note: both transforms work the same way on other database backends, so this isn't an issue.

Nice one!

Sounds like we should manually test this one on SQLite/MySQL to make sure the behaviour is the same

I've confirmed that it works on SQLite.

I haven't tested the migration on MySQL, but the query should work:

https://www.db-fiddle.com/f/gNvMtEFUYh2drgBAKjqiL5/0

Anyway, the distinction is only made for PostgreSQL anyway:

https://github.com/django/django/blob/27b07a3246bc033cd9ded01238c6dc64731cce35/django/db/models/fields/json.py#L366-L368

Hey y'all,

When I install the current main with this PR's commits on bakerydemo, with a fresh SQLite database, and run migrations for the first time, it errors out on 0070 with the following:

django.db.utils.NotSupportedError: Renaming the 'wagtailcore_pagerevision' table while in a transaction is not supported on SQLite < 3.26 because it would break referential integrity. Try adding atomic = False to the Migration class.

Thanks for the report, for some reason I did not run into that issue but it seems others have run into it too. We can continue the discussion in #8635.

wagtail/models/__init__.py

docs/reference/pages/model_reference.rst

docs/releases/4.0.md

kaedroho · 2022-05-04T10:19:53Z

wagtail/migrations/0071_populate_revision_content_type.py

+    Revision.objects.all().update(
+        base_content_type=page_type,
+        content_type_id=Cast(
+            KeyTextTransform("content_type", models.F("content")),


Nice one!

Sounds like we should manually test this one on SQLite/MySQL to make sure the behaviour is the same

wagtail/migrations/0072_alter_revision_content_type.py

wagtail/models/__init__.py

gasman

Really happy with this! Just a couple of documentation nitpicks.

docs/reference/pages/model_reference.rst

docs/releases/4.0.md

…nKey in Revision

…ecific_content_object property

laymonage force-pushed the generic-revision branch from e362c5d to 8716230 Compare April 29, 2022 14:53

laymonage commented Apr 29, 2022

View reviewed changes

laymonage marked this pull request as ready for review April 29, 2022 16:28

kaedroho requested review from gasman and kaedroho May 3, 2022 10:36

laymonage force-pushed the generic-revision branch from afa8d86 to d77d1f0 Compare May 4, 2022 05:19

laymonage commented May 4, 2022

View reviewed changes

wagtail/models/__init__.py Show resolved Hide resolved

laymonage commented May 4, 2022

View reviewed changes

wagtail/models/__init__.py Outdated Show resolved Hide resolved

kaedroho reviewed May 4, 2022

View reviewed changes

wagtail/models/__init__.py Show resolved Hide resolved

kaedroho reviewed May 4, 2022

View reviewed changes

wagtail/models/__init__.py Outdated Show resolved Hide resolved

kaedroho reviewed May 4, 2022

View reviewed changes

laymonage mentioned this pull request May 4, 2022

Extract revision-related properties and methods in Page into RevisionMixin #8482

Merged

4 tasks

lb- added status:Needs Review component:Page labels May 4, 2022

laymonage force-pushed the generic-revision branch 2 times, most recently from 22f7d4a to cee3b9a Compare May 6, 2022 03:51

kaedroho approved these changes May 6, 2022

View reviewed changes

laymonage force-pushed the generic-revision branch 7 times, most recently from d0bcba9 to bbc9d98 Compare May 16, 2022 10:09

gasman approved these changes May 16, 2022

View reviewed changes

docs/reference/pages/model_reference.rst Outdated Show resolved Hide resolved

docs/releases/4.0.md Outdated Show resolved Hide resolved

laymonage added 4 commits May 17, 2022 11:49

Rename PageRevision model to Revision

0fafedb

Turn Revision's page field into CharField

231b6ad

Rename Revision's page field into object_id

257da10

Add nullable content_type ForeignKey and content_object GenericForeig…

2c7af13

…nKey in Revision

laymonage added 22 commits May 17, 2022 11:49

Use content_type_id to help avoid unnecessary db queries

c7d1c97

Make sure to use the same content_type in is_latest_revision()

ddc54a9

Set default value for base_content_type

ebe7d69

Add content_object property to Revision

5592f0f

Update docs

75011a3

Add indexes to Revision model

c7609cb

Remove references to page where possible

d68392d

Add missing migration for verbose names

72bd678

Use base_content_type_id for queries

5f97e96

Remove unnecessary caching

0285ef8

Docs fixes

10ed5ed

Rename migration file

aa98d22

Retain cached_property for content_object so that they can be replaced

61de421

Fix page revision query in preview_revision_for_task

aa14c57

Raise warnings when accessing .page and .page_id in Revision

c6bad7b

Change .page and .page_id -> .content_object .object_id

7e7aded

Deprecate as_page_object() in favour of as_object()

28bd7ff

Replace as_page_object() -> as_object()

f480c2b

Update docs

f9e6db3

Add for_instance() method to RevisionQuerySet and Revision managers

7634d47

Fix lint issue

4c619b8

Make as_object() always use the specific content type and document sp…

dc0f41d

…ecific_content_object property

laymonage force-pushed the generic-revision branch from bbc9d98 to dc0f41d Compare May 17, 2022 04:53

Minor docs fixes

83e49d0

kaedroho merged commit 52e5abf into wagtail:main May 17, 2022

laymonage mentioned this pull request May 20, 2022

Use GenericForeignKey and GenericRevision for Revision model #8570

Closed

zerolab mentioned this pull request May 20, 2022

Fix further Wagtail main failures and tidy up GH Action/Makefile wagtail/wagtail-localize#572

Merged

chosak mentioned this pull request Jun 6, 2022

Revision model rename breaks Page revision foreign key on SQLite #8635

Closed

vanschelven mentioned this pull request Jan 5, 2023

Illegal mix of collations (utf8mb4_unicode_ci,IMPLICIT) and (utf8mb4_general_ci,IMPLICIT) in 0075_populate_latest_revision_and_revision_object_str #9477

Closed

lb- removed the status:Needs Review label Nov 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `PageRevision` with generic `Revision` model #8441

Replace `PageRevision` with generic `Revision` model #8441

laymonage commented Apr 27, 2022 •

edited

Loading

squash-labs bot commented Apr 27, 2022

laymonage left a comment

laymonage Apr 29, 2022

kaedroho May 4, 2022

gasman May 4, 2022

laymonage May 4, 2022

gasman May 4, 2022

laymonage May 5, 2022 •

edited

Loading

laymonage Apr 29, 2022 •

edited

Loading

kaedroho May 4, 2022

laymonage May 5, 2022

Scotchester Jun 3, 2022

laymonage Jun 8, 2022

kaedroho May 4, 2022

gasman left a comment

Replace PageRevision with generic Revision model #8441

Replace PageRevision with generic Revision model #8441

Conversation

laymonage commented Apr 27, 2022 • edited Loading

Footnotes

squash-labs bot commented Apr 27, 2022

Manage this branch in Squash

laymonage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laymonage May 5, 2022 • edited Loading

Choose a reason for hiding this comment

laymonage Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gasman left a comment

Choose a reason for hiding this comment

Replace `PageRevision` with generic `Revision` model #8441

Replace `PageRevision` with generic `Revision` model #8441

laymonage commented Apr 27, 2022 •

edited

Loading

laymonage May 5, 2022 •

edited

Loading

laymonage Apr 29, 2022 •

edited

Loading